fix: prep_for_nego_auth: avoid double signing redirect requests #973
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes IdentityPython#819 (again) The prepare_for_negotiated_authenticate method has sign parameter defaulting to None. The logic setting sign_redirect and sign_post does not properly handle the three-state aspects that sign has with None mixed True and False. Python evalutes `None and <any value>` as None, so as a result, None gets passed forboth sign_redirect and sign_post. However, None is interpreted by Entity._message as "sign if self.should_sign". As a result, for Redirect binding, the authentication request gets signed both in XML and in HTTP parameter (recurrence of IdentityPython#819). Fix this by passing an explicit False for exactly one of the branches (sign_post for REDIRECT binding and sign_redirect for all other bindings), passing through value of `sign` for the other branch.
|
Hi @c00kiemon5ter , I ran into the issue with double-signed authentication requests ( #819 ) again (even though it was fixed for SATOSA in IdentityPython/SATOSA#380) - and traced it to tri-state logic (None, True, False) passing I see the current code was introduced in 44d967d - I believe the logic I put in is correct, but please let me know if you think it was reintroduce the issue that 44d967d (in #834) was solving. (Sorry, I could not deduce the real intent of the change there). Please let me know if this is OK to merge or if you'd like me to make any changes. Cheers, |
|
@c00kiemon5ter , do the changes look right to you? The double signing in SAML messages in Redirect profile was breaking our use of SATOSA, I believe this fixes it without breaking anything else. Many thanks in advance for looking into it! Cheers, |
jonathanperret
added a commit
to proconnect-gouv/oidc2fer
that referenced
this pull request
Sep 18, 2025
The current version of pysaml2 (7.5.2) has an issue where AuthNRequests
are both signed in the XML and with an extra `Signature` queryparam.
This was reported initially in 2021:
IdentityPython/pysaml2#819
And it was fixed by a changed in SATOSA:
IdentityPython/SATOSA#380
But it reappeared apparently and the original reporter has a PR open
against pysaml2 that is supposed to fix it:
IdentityPython/pysaml2#973
They report that the regression was introduced in pysaml2 by
IdentityPython/pysaml2#834
We try here to pin pysaml2 to the last version before this PR was
merged. Unfortunately this is quite an old version, but from basic
testing it seems to still be compatible with the current SATOSA
version.
Hopefully this can be temporary.
jonathanperret
added a commit
to proconnect-gouv/oidc2fer
that referenced
this pull request
Sep 25, 2025
The current version of pysaml2 (7.5.2) has an issue where AuthNRequests
are both signed in the XML and with an extra `Signature` queryparam.
This was reported initially in 2021:
IdentityPython/pysaml2#819
And it was fixed by a changed in SATOSA:
IdentityPython/SATOSA#380
But it reappeared apparently and the original reporter has a PR open
against pysaml2 that is supposed to fix it:
IdentityPython/pysaml2#973
They report that the regression was introduced in pysaml2 by
IdentityPython/pysaml2#834
We try here to pin pysaml2 to the last version before this PR was
merged. Unfortunately this is quite an old version, but from basic
testing it seems to still be compatible with the current SATOSA
version.
Hopefully this can be temporary.
jonathanperret
added a commit
to proconnect-gouv/oidc2fer
that referenced
this pull request
Sep 25, 2025
The current version of pysaml2 (7.5.2) has an issue where AuthNRequests
are both signed in the XML and with an extra `Signature` queryparam.
This was reported initially in 2021:
IdentityPython/pysaml2#819
And it was fixed by a changed in SATOSA:
IdentityPython/SATOSA#380
But it reappeared apparently and the original reporter has a PR open
against pysaml2 that is supposed to fix it:
IdentityPython/pysaml2#973
They report that the regression was introduced in pysaml2 by
IdentityPython/pysaml2#834
We try here to pin pysaml2 to the last version before this PR was
merged. Unfortunately this is quite an old version, but from basic
testing it seems to still be compatible with the current SATOSA
version.
This in turn forces us to also pin xmlschema to avoid
IdentityPython/pysaml2#947
Hopefully this can be temporary.
jonathanperret
added a commit
to proconnect-gouv/oidc2fer
that referenced
this pull request
Sep 25, 2025
The current version of pysaml2 (7.5.2) has an issue where AuthNRequests
are both signed in the XML and with an extra `Signature` queryparam.
This was reported initially in 2021:
IdentityPython/pysaml2#819
And it was fixed by a changed in SATOSA:
IdentityPython/SATOSA#380
But it reappeared apparently and the original reporter has a PR open
against pysaml2 that is supposed to fix it:
IdentityPython/pysaml2#973
They report that the regression was introduced in pysaml2 by
IdentityPython/pysaml2#834
We try here to pin pysaml2 to the last version before this PR was
merged. Unfortunately this is quite an old version, but from basic
testing it seems to still be compatible with the current SATOSA
version.
This in turn forces us to also pin xmlschema to avoid
IdentityPython/pysaml2#947
Hopefully this can be temporary.
jonathanperret
added a commit
to proconnect-gouv/oidc2fer
that referenced
this pull request
Sep 25, 2025
The current version of pysaml2 (7.5.2) has an issue where AuthNRequests
are both signed in the XML and with an extra `Signature` queryparam.
This was reported initially in 2021:
IdentityPython/pysaml2#819
And it was fixed by a changed in SATOSA:
IdentityPython/SATOSA#380
But it reappeared apparently and the original reporter has a PR open
against pysaml2 that is supposed to fix it:
IdentityPython/pysaml2#973
They report that the regression was introduced in pysaml2 by
IdentityPython/pysaml2#834
We try here to pin pysaml2 to the last version before this PR was
merged. Unfortunately this is quite an old version, but from basic
testing it seems to still be compatible with the current SATOSA
version.
This in turn forces us to also pin xmlschema to avoid
IdentityPython/pysaml2#947
Hopefully this can be temporary.
jonathanperret
added a commit
to proconnect-gouv/oidc2fer
that referenced
this pull request
Sep 25, 2025
The current version of pysaml2 (7.5.2) has an issue where AuthNRequests
are both signed in the XML and with an extra `Signature` queryparam.
This was reported initially in 2021:
IdentityPython/pysaml2#819
And it was fixed by a changed in SATOSA:
IdentityPython/SATOSA#380
But it reappeared apparently and the original reporter has a PR open
against pysaml2 that is supposed to fix it:
IdentityPython/pysaml2#973
They report that the regression was introduced in pysaml2 by
IdentityPython/pysaml2#834
We try here to pin pysaml2 to the last version before this PR was
merged. Unfortunately this is quite an old version, but from basic
testing it seems to still be compatible with the current SATOSA
version.
This in turn forces us to also pin xmlschema to avoid
IdentityPython/pysaml2#947
Hopefully this can be temporary.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #819 (again)
The prepare_for_negotiated_authenticate method has sign parameter defaulting to None.
The logic setting
sign_redirectandsign_postdoes not properly handle the three-state aspects thatsignhas withNonemixed withTrueandFalse.Python evalutes
None and <any value>asNone, so as a result,Nonegets passed for bothsign_redirectandsign_post.However,
Noneis interpreted byEntity._messageas "signif self.should_sign".As a result, for Redirect binding, the authentication request gets signed both in XML and in HTTP parameter (recurrence of #819).
Fix this by passing an explicit
Falsefor exactly one of the branches (sign_post for REDIRECT binding and sign_redirect for all other bindings), passing through value ofsignfor the other branch.Description
The feature or problem addressed by this PR
Fix double signing of of Authentication requests with redirect binding.
What your changes do and why you chose this solution
Fix logic to avoid passing
Nonefor bothsign_postandsign_redirect(as they both get interpreted as "sign ifshould_sign)Checklist